Support snapshot in rbdplugin #61

sngchlko · 2018-08-08T07:26:24Z

Snapshot is one of the added features in CSI 3.0 spec.
This PR implements functions about the snapshot feature in rbdplugin.

What has been added:

CreateSnapshot:
- creates a snapshot with rbd snap create
- the snapshot is protected with rbd snap protect for cloning
DeleteSnapshot:
- the snapshot is unprotected with rbd snap unprotect
- remove snapshot with rbd snap rm
ListSnapshots:
- get all snapshots from the cache.
- if requested source volume id exists, get all snapshots which have same source volume id
- if requested snapshot id exists, get a specific snapshot
Store snapshot information into the persistent file
- use the same method like storing volume information but store in the different directory

What has been changed:

CreateVolume:
- if this request is for restoring snapshot called by snapshot controller, creates a volume from the snapshot with rbd clone

gman0 · 2018-08-08T11:08:17Z

pkg/cephfs/nodeserver.go

@@ -212,3 +212,11 @@ func (ns *nodeServer) NodeUnstageVolume(
 	*csi.NodeUnstageVolumeResponse, error) {
 	return nil, status.Error(codes.Unimplemented, "")
 }
+
+func (ns *nodeServer) NodeGetInfo(


I believe we could do without overriding NodeGetInfo since it's already in csicommon.DefaultNodeServer. See https://github.com/kubernetes-csi/drivers/blob/master/pkg/csi-common/nodeserver-default.go#L47

And it also sets NodeId.

You are right. I will remove this.

This reverts commit c93466b.

rootfs · 2018-08-08T13:36:44Z

pkg/rbd/controllerserver.go

+			}
+
+			snapshotID := snapshot.GetId()
+			if snapshotID == "" {


nit: len(snapshotID) == 0, consistent with the rest of the tests

rootfs · 2018-08-08T13:38:13Z

pkg/rbd/controllerserver.go

+	}
+	rbdSnap.VolName = rbdVolume.VolName
+	rbdSnap.SnapName = snapName
+	snapshotID := "csi-rbd-snapshot-" + uniqueID


nit: add image name

rootfs · 2018-08-08T13:39:26Z

pkg/rbd/controllerserver.go

+	rbdSnap.SizeBytes = rbdVolume.VolSize
+
+	err = createSnapshot(rbdSnap, req.GetCreateSnapshotSecrets())
+	// if we already have the snapshot, return the snapshot


any reason the snapshot is created more than once? the unique id should prevent it...

I think this is to make sure if plugin gets the create snapshot request for the same snapshot more than once, it won't create it again. This is to address the idempotent requirement of CSI spec.

I think so. CSI Spec explained the details.
This operation MUST be idempotent. If a snapshot corresponding to the specified snapshot name is already successfully cut and uploaded (if upload is part of the process) and is compatible with the specified source_volume_id and parameters in the CreateSnapshotRequest, the Plugin MUST reply 0 OK with the corresponding CreateSnapshotResponse.

rootfs · 2018-08-08T13:41:09Z

pkg/rbd/controllerserver.go

+	rbdSnap.CreatedAt = time.Now().UnixNano()
+
+	// Storing snapInfo into a persistent file.
+	if err := persistSnapInfo(snapshotID, path.Join(PluginFolder, "controller-snap"), rbdSnap); err != nil {


also see
#62 (comment)

I agree using local storage is the problem.
I think we need external storage to share volume or snapshot information between rbd-plugins.
How about using etcd to store information?
cc: @gman0

@sngchlko @rootfs let's discuss this issue after this is done here #66

rootfs · 2018-08-08T13:41:33Z

pkg/rbd/controllerserver.go

+	}
+
+	snapshotID := req.GetSnapshotId()
+	if snapshotID == "" {


nit: len(snapshotID) == 0

rootfs · 2018-08-08T13:45:40Z

pkg/rbd/rbd_util.go

@@ -391,3 +461,132 @@ func getRBDVolumeByName(volName string) (rbdVolume, error) {
 	}
 	return rbdVolume{}, fmt.Errorf("volume name %s does not exit in the volumes list", volName)
 }
+
+func getRBDSnapshotByName(snapName string) (rbdSnapshot, error) {


nit: (*rbdSnapshot, error)

rootfs · 2018-08-08T13:46:23Z

pkg/rbd/rbd_util.go

+		return err
+	}
+	glog.V(4).Infof("rbd: snap protect %s using mon %s, pool %s id %s key %s", image, pOpts.Monitors, pOpts.Pool, RBDUserID, key)
+	args := []string{"snap", "protect", "--pool", pOpts.Pool, "--snap", snapID, image, "--id", RBDUserID, "-m", mon, "--key=" + key}


is RBDUserID sufficient to create the snapshot or do we need admin id for it?

ooh, i see, it is a hardcoded admin. This needs to change, eventually.

rootfs · 2018-08-08T13:46:48Z

pkg/rbd/rbd_util.go

+		return err
+	}
+	glog.V(4).Infof("rbd: snap create %s using mon %s, pool %s id %s key %s", image, pOpts.Monitors, pOpts.Pool, RBDUserID, key)
+	args := []string{"snap", "create", "--pool", pOpts.Pool, "--snap", snapID, image, "--id", RBDUserID, "-m", mon, "--key=" + key}


rootfs · 2018-08-08T13:56:11Z

pkg/rbd/rbd.go

-	// Since "controller" folder exists, it means the rbdplugin has already been running, it means
-	// there might be some volumes left, they must be re-inserted into rbdVolumes map
-	loadExVolumes()
+	for _, f := range files {


hold a lock here

rootfs · 2018-08-08T13:56:49Z

pkg/rbd/rbd_util.go

@@ -376,6 +402,50 @@ func deleteVolInfo(image string, persistentStoragePath string) error {
 	return nil
 }

+func persistSnapInfo(snapshot string, persistentStoragePath string, snapInfo *rbdSnapshot) error {
+	file := path.Join(persistentStoragePath, snapshot+".json")


hold a lock here

rootfs · 2018-08-08T13:56:58Z

pkg/rbd/rbd_util.go

+}
+
+func deleteSnapInfo(snapshot string, persistentStoragePath string) error {
+	file := path.Join(persistentStoragePath, snapshot+".json")


hold a lock

rootfs · 2018-08-08T13:57:58Z

pkg/rbd/controllerserver.go

+	// Storing snapInfo into a persistent file.
+	if err := persistSnapInfo(snapshotID, path.Join(PluginFolder, "controller-snap"), rbdSnap); err != nil {
+		glog.Warningf("rbd: failed to store sanpInfo with error: %v", err)
+		return nil, err


delete snapshot

rootfs · 2018-08-08T14:00:34Z

@sngchlko thanks for getting this done here

@xing-yang can you take a look? thanks

rootfs · 2018-08-08T16:17:33Z

@sngchlko just to confirm, does rbd snapshot and restore work on your krbd test without any special rbd image features?

xing-yang · 2018-08-09T03:15:38Z

It will be great if you can test this driver with the snapshot controller PR: kubernetes-csi/external-snapshotter#7

sngchlko · 2018-08-09T07:07:58Z

@rootfs I had to use layering feature to create the new volume from the snapshot. I think it will be great to check image features at the time of creating the snapshot.

@xing-yang External-snapshotter sends CreateSnapshotSecrets as nil at the time of creating the snapshot. Actually, I tested with my own implemented csi-snapshot-contoller based on snapshot-controller in kubernetes-incubator. I hope to test on external-snapshotter soon.

rootfs · 2018-08-09T13:31:36Z

@sngchlko thanks!

xing-yang · 2018-08-09T13:43:48Z

@sngchlko We need to fix the "CreateSnapshotSecrets as nil" issue in the External-snapshotter PR. Thanks for your feedback! Please let us know if you see other issues in that PR.

dillaman · 2018-08-16T13:20:34Z

pkg/rbd/controllerserver.go

+		}
+	} else {
+		glog.V(4).Infof("create snapshot %s", snapName)
+		err = protectSnapshot(rbdSnap, rbdSnap.AdminId, req.GetCreateSnapshotSecrets())


Sorry for the late comment, but why are you protecting the snapshot? Snapshot protection was only required to support RBD cloning, but with Mimic and its new clone v2 support, snapshot protection is on its way to being deprecated. Since this is all new functionality, it would be great if we could start w/ a clean implementation.

thanks @dillaman

@sngchlko do you have a chance to consider this?

@dillaman Thanks for the review. Good point!!
But I think only supporting snapshot with Mimic is not good.
In my case, I have to use the luminous version as I installed the ceph with a rook project which supports only luminous now.
And people who want to use snapshot feature in CSI must upgrade to Mimic.
How about using protect operation according to version?

@sngchlko Like I said, the "protect" operation is only relevant for cloning images (using older versions of Ceph). Creating a snapshot only requires the "rbd snap create" command.

@dillaman You are right. But I think we need protecting snapshot for creating a new image from the snapshot. CSI spec seems to require creating new volume at the time of restoring the snapshot. AFAIK, cloning the snapshot requires protecting the snapshot.

@sngchlko I'm not familiar w/ the CSI spec, can you provide a link? I see [1] but it states revert isn't part of it. I would really like to avoid going down the tangled code of the OpenStack world due to how snapshots and clones needed to be handled before, which is why we added clone v2. Since this is all new functionality, it would be great if we could start off on a clean footing and avoid having to handle things like renaming protected snapshots "XYZ_deleteme" and images as "XYZ_deleteme" if they have a linked clone.

[1] https://docs.google.com/document/d/1oXVuDTNhXerr1UJ48tQrxF9J6vbm2LHVVC3z28655Qo/edit#heading=h.aqok97b5hs7t

@dillaman Sure, you can find VolumeContentSource for restoring the snapshot in CSI spec. If VolumeContentSource has something, it creates the new volume from the snapshot.
I think I can't determine about supporting snapshot only Mimic.
@rootfs, @gman0 What do you think about this?

@sngchlko Thanks. If the only use-case for snapshots right now is to restore and if restore requires creating new volumes (instead of rolling back), perhaps the short-term (pre-Mimic) thing to do would be to protect the snapshot upon create from snapshot, flatten the new volume, and unprotect the snapshot. Mimic and later clients can just directly clone the snapshot into a new image. --- or just only support creating images from a snapshot if you are using Mimic or later and enjoy the simpler CSI code that doesn't need to manage snapshot lifecycles.

@dillaman I fully understand your concerns. I created #70. Thanks!

dillaman · 2018-08-16T13:22:33Z

pkg/rbd/rbd_util.go

@@ -376,18 +432,191 @@ func deleteVolInfo(image string, persistentStoragePath string) error {
 	return nil
 }

-func getRBDVolumeByID(volumeID string) (rbdVolume, error) {
+func persistSnapInfo(snapshot string, persistentStoragePath string, snapInfo *rbdSnapshot) error {


Use "rbd image-meta get/set" to associate the snapshot data directly within the RBD image instead of storing as a local file

Support snapshot in rbdplugin

BUG 1974816: util: allow any KMS connection to be configured by ConfigMap

BUG 2030742: rbd: reset dummy image id

Implement NodeGetInfo for csi spec 3.0

c93466b

sngchlko force-pushed the support-snapshot-in-rbdplugin branch from 941119c to 05ecabf Compare August 8, 2018 07:52

Support snapshot feature in rbdplugin

b1ccdbb

sngchlko force-pushed the support-snapshot-in-rbdplugin branch from 05ecabf to b1ccdbb Compare August 8, 2018 08:16

gman0 reviewed Aug 8, 2018

View reviewed changes

Revert "Implement NodeGetInfo for csi spec 3.0"

f0fba12

This reverts commit c93466b.

rootfs reviewed Aug 8, 2018

View reviewed changes

pkg/rbd/controllerserver.go Outdated

}

snapshotID := req.GetSnapshotId()

if snapshotID == "" {

Copy link

Member

rootfs Aug 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: len(snapshotID) == 0

rootfs reviewed Aug 8, 2018

View reviewed changes

rootfs closed this Aug 8, 2018

rootfs reopened this Aug 8, 2018

sngchlko added 4 commits August 9, 2018 22:06

fix nit

7d90783

Refactoring using users

b0e68a5

remove the snapshot if can't store snapshot information

4312907

check snapshot feature

38aa575

rootfs merged commit 43b9f9a into ceph:master Aug 9, 2018

gman0 mentioned this pull request Aug 9, 2018

Move to CSI 0.3.0 #47

Closed

dillaman reviewed Aug 16, 2018

View reviewed changes

wilmardo pushed a commit to wilmardo/ceph-csi that referenced this pull request Jul 29, 2019

Merge pull request ceph#61 from sngchlko/support-snapshot-in-rbdplugin

26fd16c

Support snapshot in rbdplugin

nixpanic pushed a commit to nixpanic/ceph-csi that referenced this pull request Jul 6, 2021

Merge pull request ceph#61 from nixpanic/ocs/4.7/vault

8d0e647

BUG 1974816: util: allow any KMS connection to be configured by ConfigMap

Madhu-1 pushed a commit to Madhu-1/ceph-csi that referenced this pull request Dec 23, 2021

Merge pull request ceph#61 from Madhu-1/bz-2030742-1

575c501

BUG 2030742: rbd: reset dummy image id

Support snapshot in rbdplugin #61

Support snapshot in rbdplugin #61

Conversation

sngchlko commented Aug 8, 2018

gman0 Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rootfs commented Aug 8, 2018

rootfs commented Aug 8, 2018

xing-yang commented Aug 9, 2018

sngchlko commented Aug 9, 2018

rootfs commented Aug 9, 2018

xing-yang commented Aug 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gman0 Aug 8, 2018 •

edited

Loading